CXH-2379: fix grant/revoke idempotency for DDL-based engines - #151
CXH-2379: fix grant/revoke idempotency for DDL-based engines#151al-conductorone wants to merge 16 commits into
Conversation
Validation-query "no rows" now wraps ErrQueryAffectedZeroRows so the provisioning layer's errors.Is check reports GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task. DDL dialects (e.g. Db2) whose GRANT/REVOKE raise an error rather than affecting rows can only signal prior state through validation_queries, which previously landed on the failing path. Adds regression tests driving Grant/Revoke end-to-end over in-memory sqlite for both the already-applied (idempotent) and apply cases.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXH-2379: fix grant/revoke idempotency for DDL-based enginesBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Review SummaryScanned the full PR diff for security and correctness: the Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
A validation query returning no rows now maps to ErrQueryAffectedZeroRows (reported as GrantAlreadyExists / GrantAlreadyRevoked) only on DDL-based engines (Db2), which don't report rows-affected. Other engines keep using validation queries as existence preconditions that fail loudly, so a grant against a missing user or role is no longer silently reported as success. This also restores the grant_replace abort behavior on those engines: a replaced-grant revoke whose validation returns no rows returns a plain error instead of the sentinel, so GrantReplaced is not emitted. Document the engine-specific ValidationQueries semantics and add a test covering the non-DDL loud-failure path.
Mirror TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly on the revoke path: on a non-DB2 engine, a revoke validation query returning no rows is a failed precondition, so Revoke returns an error with nil annotations rather than GrantAlreadyRevoked. Pins the false branch of validationNoRowsMeansIdempotent() for RunProvisioningQueriesWithExecutor.
| // don't report rows-affected, so the validation query is the only zero-effect signal | ||
| // available. Engines that report rows-affected keep using validation queries as | ||
| // existence preconditions that fail loudly. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { |
There was a problem hiding this comment.
[Review] why only DB2?
The bug report and this PR's own description frame this as a general "DDL-based engines" problem, not a DB2-only thing — and examples/oracle-test.yml has the exact same DDL-shaped GRANT ... TO / REVOKE pattern, so Oracle is probably exposed to the same bug. Hardcoding database.DB2 here means it's still broken there.
Could totally be intentional (only fix what's actually verified, per the stability-first vibe of this repo) — if so no action needed, just curious if that's the reasoning or if it's worth a quick follow-up ticket for Oracle/MSSQL/HDB/Vertica too.
There was a problem hiding this comment.
Confirmed: Oracle got added (case database.DB2, database.Oracle: in validationNoRowsMeansIdempotent()), so this thread's question is answered — but it lands on exactly the blocking concern @mateoHernandez123 raised separately: Oracle has no build tag (pkg/database/oracle/*.go is plain package oracle, unlike Db2's //go:build db2), so it ships in every default binary, and there's no config-level opt-in flag anywhere in pkg/bsql/config.go. That means every existing Oracle deployment using validation_queries" as an existence precondition (the same pattern this repo's own postgres-test.ymldemonstrates as normal) now silently getsGrantAlreadyExists/GrantAlreadyRevoked` on a missing or mistyped principal instead of a loud error — with zero opt-in. Db2 could take this unconditionally because it's opt-in behind a build tag; Oracle can't. This still looks unresolved and blocking to me.
There was a problem hiding this comment.
reverted oracle, db2-only again. it's a follow-up behind a per-config opt-in since oracle ships default-on.
- Extract shared runValidationQueries helper so the grant and revoke validation loops stop drifting (the copies had diverged on result.Close). - Warn in the ValidationQueries doc comment that DDL-engine authors must not reuse validation_queries as an existence precondition, since a no-rows result is reported as idempotent success and would mask real failures. - Preserve annotations returned by RunGrantProvisioning in the already-exists branch so a GrantReplaced from a committed grant_replace revoke survives.
On the transactional grant path, RunGrantProvisioning returns the zero-rows sentinel before commit, so the deferred rollback undoes any grant_replace revoke. Grant reused those annotations, reporting GrantReplaced for a removal the database no longer reflects. Keep the returned annotations only on the no_transaction path, where the replace already committed; otherwise return a fresh GrantAlreadyExists. Adds regression tests for both the rolled-back (no GrantReplaced, old grant survives) and committed (GrantReplaced, old grant gone) paths.
On Db2 a grant_replace revoke whose validation query returns no rows swallows ErrQueryAffectedZeroRows and still reports GrantReplaced: the old grant is already gone, which is the state a replace aims for. Document this at the guard, cover it with a DB2 test, and add a validation_queries section to docs/db2.md warning against using them as existence preconditions on Db2.
Replace os.Exit(1) in main.go with exit.LogExit(err) so an auth failure exits with the mapped gRPC status code instead of a bare 1, letting the CI sync-test auth-error check actually assert.
Address PR review on the Oracle idempotency change: - Fix the validationNoRowsMeansIdempotent doc comment. Re-GRANT on Oracle succeeds silently; ORA-01951 is a REVOKE-only error, so the old wording claiming a repeat GRANT raises ORA-01951 was wrong. - Add docs/provisioning.md, an engine-neutral home for the no-rows-means- idempotent behavior covering both DDL engines (Db2 and Oracle), so Oracle operators can find the existence-precondition warning. - Trim the Db2-scoped section in docs/db2.md to point at the shared doc and drop the now-stale "different meaning than every other (pure-Go) engine" claim, since Oracle (also pure-Go) now shares the behavior.
…nt-and-revoke-idempotency-for-ddl-based
Pairs with the exit.LogExit change: Validate wrapped the ping error plainly, so exit mapped auth failures to Unknown(2). database.AuthError maps SQLSTATE class 28 (Postgres/Redshift/Vertica/etc.) and MySQL 1045 to codes.Unauthenticated.
…evoke On a DDL engine a revoke whose validation_queries return no rows short- circuits before any revoke runs. RunRevokeProvisioning still ran the principal_exists_check probe, so a mistyped principal_id (validation and probe both empty) falsely reported a still-present principal as deleted, contradicting the PrincipalExistsCheck contract. Add a distinct ErrValidationNoRows sentinel (wrapping ErrQueryAffectedZeroRows so idempotency reporting is unchanged) and skip the exists probe when the zero-rows result came from validation rather than the revoke queries running. Also: reword the grant_replace zero-rows comment to cover both sentinel sources (not just DDL), include the failing query in the loud validation error, and link docs/provisioning.md from README. Adds a regression test.
AuthError now takes the failing database name so a multi-DB config shows which handle rejected the credentials, and its doc comment records that coverage is limited to SQLState-reporting drivers plus MySQL (Oracle/Db2/ MSSQL/HDB fall through to a generic ping error, not Unauthenticated).
Addressed across commits a439489..8c0917e: idempotency gated to Db2+Oracle only (Oracle verified live), GrantReplaced-on-rollback fixed, principal-exists probe skipped on validation-sourced no-rows, comment/doc accuracy fixes, docs/provisioning.md + README link, DB-name in auth error + driver-coverage note. Oracle inclusion is intended and documented.
| // Skip the probe when the zero-rows came from a validation query (DDL engines): no | ||
| // revoke ran, so a no-rows exists-check would falsely report the principal deleted | ||
| // "as a side effect of the revoke" when it may still be present. | ||
| if existsCheck != nil && !fromValidation { |
There was a problem hiding this comment.
🟡 Suggestion: the guard is right, but the RunRevokeProvisioning doc comment above (lines 464-467) still promises the function "still commits and probes ... combined with ResourceDeleted when the principal is also gone, so retried revokes still surface the deletion." That contract no longer holds on Db2/Oracle configs that use validation_queries — a retried revoke short-circuits at validation and never reports a cascaded principal deletion. Please update that doc block (and the matching note at pkg/bsql/provisioning.go:156-157) so the trade-off is visible from the contract. (medium confidence)
| if !valid { | ||
| return fmt.Errorf("validation query returned no rows") | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| return ErrValidationNoRows | ||
| } | ||
| return fmt.Errorf("validation query %q returned no rows", q) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the two branches are now asymmetric in diagnosability. The non-DDL branch gained %q with the offending query, but the DDL branch returns a bare ErrValidationNoRows and emits no log at all — so on Db2/Oracle the exact hazard the new docs/provisioning.md warns about (a mistyped principal_id making a validation query return no rows, reported to C1 as GrantAlreadyExists/GrantAlreadyRevoked) leaves nothing in the logs to diagnose it. Consider a l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) before the return, or wrapping the query into the sentinel with fmt.Errorf("validation query %q returned no rows: %w", q, ErrValidationNoRows) — errors.Is against both sentinels keeps working either way. (medium confidence)
Adding Oracle to validationNoRowsMeansIdempotent() was a default-on breaking change: unlike Db2 (opt-in behind the db2 build tag), Oracle ships in every default build, so existing Oracle configs that use validation_queries as loud existence preconditions would silently start reporting GrantAlreadyExists / GrantAlreadyRevoked on a missing or mistyped principal. Gate back to Db2 only; Oracle stays a follow-up pending a per-config opt-in. - validationNoRowsMeansIdempotent(): Db2 only again; doc names the build-tag asymmetry so the gate isn't widened again by accident. - Engine-gate test asserts Oracle (and every non-Db2 engine) is false, guarding re-introduction. - Restore Db2-only wording in config.go, docs/db2.md, docs/provisioning.md. - runValidationQueries: on the idempotent path, wrap the query into the sentinel and Warn-log it, so a swallowed no-rows validation stays diagnosable (dropped when the shared helper was extracted).
…x-grant-and-revoke-idempotency-for-ddl-based # Conflicts: # pkg/connector/connector.go # pkg/database/autherror.go # pkg/database/autherror_test.go
| - **Account Provisioning**: Define schemas and credential options for user creation | ||
| - **Entitlements**: Permissions and roles that can be granted to resources | ||
| - **Provisioning Actions**: SQL queries for granting/revoking entitlements | ||
| - **Provisioning Actions**: SQL queries for granting/revoking entitlements; see [docs/provisioning.md](docs/provisioning.md) for `validation_queries` semantics (including the DDL-engine no-rows-means-idempotent behavior on Db2 and Oracle) |
There was a problem hiding this comment.
🟡 Suggestion: This says the no-rows-means-idempotent behavior applies to "Db2 and Oracle", but validationNoRowsMeansIdempotent() (pkg/bsql/query.go:625) gates on database.DB2 only, docs/provisioning.md explicitly states Oracle "still fail[s] loudly like everyone else", and TestValidationNoRowsMeansIdempotent_EngineGate asserts Oracle: false. An Oracle operator reading this line would write validation_queries expecting idempotent no-rows and instead get a hard failure. Drop "and Oracle".
| } | ||
| return annotations.New(&v2.GrantAlreadyExists{}), nil | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
🟡 Suggestion: The new comment above correctly reasons that on the no_transaction path a grant_replace revoke has already committed — but that reasoning only gets applied to the zero-rows branch. On this generic error path (e.g. the main grant query fails with a constraint violation after the replace revoke committed), anno is discarded and nil, err is returned, so the already-executed removal is never surfaced. The SDK likely drops annotations on an error return anyway, so the practical fix is a l.Warn here when provisioningConfig.Grant.NoTransaction && anno carries GrantReplaced, recording that the replace committed but the grant failed.
| // A zero-rows sentinel means the replace revoke had nothing to remove: either | ||
| // its validation query found no rows on a DDL engine, or the revoke queries | ||
| // matched nothing on any engine. Either way the old grant is already gone, the | ||
| // state a replace aims for, so report GrantReplaced. Any other error aborts. |
There was a problem hiding this comment.
grant_replace reports the old grant as removed even when no revoke statement ran.
Context for whoever reads this cold: grant_replace means "before granting X, revoke the grant Y that this query returns". GrantReplaced is the annotation that tells ConductorOne "Y no longer exists", and C1 drops Y from its graph on that word alone — it does not re-check the database.
This branch swallows every ErrQueryAffectedZeroRows and then emits GrantReplaced unconditionally, but after this PR that sentinel covers two different situations:
- the revoke statements ran and matched nothing → Y really is gone,
GrantReplacedis accurate; ErrValidationNoRows(Db2) → the validation query short-circuited before any revoke statement ran, so whether Y is gone depends entirely on that validation query honoring the contract indocs/provisioning.md.
The PR already draws exactly this distinction on the revoke path: runRevokeQueries threads fromValidation out specifically so RunRevokeProvisioning can skip the principal-exists probe and avoid reporting a deletion that never happened (query.go L492-496). The same reasoning applies here, one level up: a validation short-circuit is not evidence that the old grant is gone.
Either resolution works for me:
- gate the annotation on
!errors.Is(err, ErrValidationNoRows)— the failure mode is safe, C1 keeps Y and the next sync corrects it; or - keep the current behavior and say in the comment that it is load-bearing on the Db2
validation_queriescontract, so a reader understands the guarantee comes from config, not from the code.
What I would avoid is leaving the two paths asymmetric with no note, since the next reader will reasonably assume the fromValidation guard covers this call site too.
| withGrantReplaceConfig(s, true) // no_transaction: the replace stands on its own | ||
| revoke := s.config.StaticEntitlements[0].Provisioning.Revoke | ||
| revoke.ValidationQueries = []string{ | ||
| `SELECT 1 FROM user_roles WHERE user_id = ?<user_id> AND role = 'does-not-exist'`, |
There was a problem hiding this comment.
This fixture encodes the config the new docs tell users not to write, and then asserts the result as correct.
docs/provisioning.md, added in this PR, is explicit: on Db2 a revoke validation_query must answer "is there work to do?" — for a revoke, "is the old membership present?", so that no rows genuinely means "already revoked". It also warns that using it as an existence check silently masks a bad principal or role.
Here the old membership is viewer, but the validation query asks about role = 'does-not-exist', so it can never match in any database state. That is the masking case the doc warns about, not the idempotency case the PR is adding.
That is also why the test can assert two things that cannot both hold in a correct run: GrantReplaced for the viewer grant (L154-156) and viewer still present in the table (L159). Downstream that means C1 drops the grant while the row survives upstream, so the next sync re-creates it and the grant flaps between syncs.
Concrete suggestion: keep the validation query pointed at the real membership (role = 'viewer') and simply don't insert the viewer row in the setup. No rows then genuinely means "already revoked", GrantReplaced is accurate, and the test proves the idempotency reporting this PR is about. If you also want coverage for the misconfigured-query case, a second test asserting today's behavior and named for it (e.g. ...ValidationQueryIsExistenceCheck...) would make the trade-off explicit instead of implicit.
| // DDL engines are a follow-up: they ship default-on, so flipping this would break existing | ||
| // configs that use validation_queries as loud preconditions, and need a per-config opt-in first. | ||
| func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool { | ||
| return s.dbEngine == database.DB2 |
There was a problem hiding this comment.
Non-blocking, and mostly about framing rather than this function.
Keeping the gate Db2-only looks right to me, and docs/provisioning.md is clear that Oracle keeps failing loudly until there is a per-config opt-in. Flipping it globally would silently reinterpret existing configs that use validation_queries as preconditions, which is a worse outcome than the current gap.
The mismatch is in the description: the title says "DDL-based engines" (plural) and the body "DDL-based databases (such as Db2)", while the behavior reaches exactly one engine, itself behind the db2 build tag. Someone hitting repeat-grant failures on Oracle will read the title, assume this shipped for them, and re-open the same investigation.
Could you narrow the title to Db2 and link the Oracle follow-up in the body? One thing worth capturing in that follow-up: the DDL-vs-DML label is not the deciding factor, the driver's rows-affected reporting is. The pinned go-ora returns a real RowsAffected for DML (command.go L300-L302), so the Oracle decision needs an observed zero-effect case rather than the category name.
| if !valid { | ||
| return fmt.Errorf("validation query returned no rows") | ||
| if s.validationNoRowsMeansIdempotent() { | ||
| l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) |
There was a problem hiding this comment.
Warn on what this PR is making the supported happy path.
This line fires on every idempotent grant or revoke on Db2 — the case the PR exists to support — and the message itself says "treating as idempotent success". Connector logs surface to operators, so re-running a grant that worked as designed produces warnings, and the level stops carrying information: if expected outcomes warn, real warnings stop standing out.
I realize the repo does use l.Warn elsewhere (resources.go has around a dozen), so this is not me pushing a foreign style. Those are one-shot configuration and parse problems where someone genuinely should look at the config. This one is per-request expected control flow. Debug is the level for that, and the fmt.Errorf on the next line already carries the query text for anyone debugging a specific request.
l.Debug(...) with the same fields keeps the diagnostic value and drops the noise.
4 participants
Repeat grant or revoke requests against DDL-based databases (such as Db2) no longer fail; the connector now recognizes when access is already in the requested state and reports the operation as a successful no-op.